Tweak the `PollEvented::deregister` signature #55

alexcrichton · 2017-12-05T16:57:27Z

This commit changes the PollEvented::deregister signature from

fn deregister(self, handle: &Handle) -> io::Result<()>

to

fn deregister(&self) -> io::Result<()>

Now that the handles are Send and Sync there's no longer any need to pass it
in (it's already stored in the PollEvented itself). Additionally this switches
to &self instead of self to allow reclamation of the internal resources if
necessary.

This PR is based on #54, only the last commit needs to be reviewed.

The `Handle` type is now `Send` and `Sync` so the `Remote` type no longer needs to exist.

No need for oneshot shenanigans as now we'll always have the `Handle` available to us regardless of what thread we're on to associate a new socket

This is no longer needed now that the public-facing `CoreId` has been removed

This commit renames the various constructors of networking types to have a `_std` suffix instead of a smorgasboard of other suffixes, canonicalizing on `_std` as the suffix for constructors which take the libstd corresponding types.

This commit is targeted at solving tokio-rs/tokio-core#12 and incorporates the solution from tokio-rs/tokio-core#17. Namely the `need_read` and `need_write` functions on `PollEvented` now return an error when the connected reactor has gone away and the task cannot be blocked. This will typically naturally translate to errors being returned by various connected I/O objects and should help tear down the world in a clean-ish fashion.

This should allow configuration over what reactor accepted streams go on to by giving back a libstd-bound object that can then be used later in conjunction with `TcpStream::from_std`.

This commit changes the `PollEvented::deregister` signature from fn deregister(self, handle: &Handle) -> io::Result<()> to fn deregister(&self) -> io::Result<()> Now that the handles are `Send` and `Sync` there's no longer any need to pass it in (it's already stored in the `PollEvented` itself). Additionally this switches to `&self` instead of `self` to allow reclamation of the internal resources if necessary.

cramertj · 2017-12-05T19:30:27Z

Nice! Now that it no longer requires any external parameters, should deregister be removed then in favor of a Drop impl? tokio-rs/mio#753 brought up some issues that could be resolved by doing this.

alexcrichton · 2017-12-05T21:49:23Z

@cramertj perhaps yeah, but I think we'd still want deregister anyway b/c that way (on Unix at least) you could migrate objects between event loops (in theory at least). I've also personally been under the assumption so far that if you drop the PollEvented it internally drops the E which at the OS level implicitly performs deregistration so an explicit call isn't necessary.

That being said tokio-rs/mio#753 would of course change the calculus here quite a lot!

cramertj · 2017-12-05T21:57:58Z

@alexcrichton

that way (on Unix at least) you could migrate objects between event loops

I'd assume in order to do that you'd need a register function of some sort which takes a new Handle. If that's the case, you could call deregister internally prior to registering with the new event loop.

I've also personally been under the assumption so far that if you drop the PollEvented it internally drops the E which at the OS level implicitly performs deregistration so an explicit call isn't necessary.

That makes sense, but I think the conversation in tokio-rs/mio#753 and tokio-rs/mio#361 turned up that implicit deregistration doesn't work right if the underlying file descriptor has been duplicated.

alexcrichton · 2017-12-05T22:04:21Z

@cramertj oh I was also thinking we'd have something like into_inner eventually.

but I think the conversation in tokio-rs/mio#753 and tokio-rs/mio#361 turned up that implicit deregistration doesn't work right if the underlying file descriptor has been duplicated.

True, but we don't expose any apis that duplicate underlying file descriptors (precisely for this reason!) so if you get into that situation you're sort of "on your own" at that point anyway.

FWIW I believe the original motivation for a method like this was to deregister file descriptors in response to external runtimes like libcurl provides. Looking at tokio-curl today it doesn't actually use this though so maybe it migrated away at some point!

Regardless though I think for low-level functionality we'll want to retain functionality like this as it seems inevitable to come up at some point, although I could also imagine tweaking the signature of deregister or having more stern warnings about using the PollEvented after deregister has been called.

cramertj · 2017-12-05T22:08:00Z

@alexcrichton Okay! Seeing as tokio-rs/tokio-core#282 gets around this by not using PollEvented at all, I will plan to continue doing my own thing for Fuchsia and trust your judgement concerning other platforms :)

alexcrichton · 2017-12-05T22:08:56Z

Heh, true!

alexcrichton added 7 commits December 5, 2017 08:15

Remove the Remote type

7c768fc

The `Handle` type is now `Send` and `Sync` so the `Remote` type no longer needs to exist.

Remove TcpListener::pending_accept field

329bca1

No need for oneshot shenanigans as now we'll always have the `Handle` available to us regardless of what thread we're on to associate a new socket

Remove NEXT_LOOP_ID

259996d

This is no longer needed now that the public-facing `CoreId` has been removed

Rename networking constructors with _std

8fcce95

This commit renames the various constructors of networking types to have a `_std` suffix instead of a smorgasboard of other suffixes, canonicalizing on `_std` as the suffix for constructors which take the libstd corresponding types.

Add TcpListener::accept_std as a method

2e58422

This should allow configuration over what reactor accepted streams go on to by giving back a libstd-bound object that can then be used later in conjunction with `TcpStream::from_std`.

alexcrichton mentioned this pull request Dec 5, 2017

Blanket rename Core to Reactor #56

Merged

carllerche merged commit 4606279 into tokio-rs:new-crate Dec 5, 2017

alexcrichton deleted the change-deregister branch December 5, 2017 21:47

binomial0 mentioned this pull request Jun 17, 2019

tokio-async-await broken on latest nightly #1154

Closed

renovate bot mentioned this pull request Jul 27, 2023

fix(deps): update all non-major dependencies jzeuzs/website-screenshot#55

Open

1 task

zRegle mentioned this pull request May 31, 2024

Segment fault at poll_future #6605

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak the `PollEvented::deregister` signature #55

Tweak the `PollEvented::deregister` signature #55

alexcrichton commented Dec 5, 2017 •

edited

Loading

cramertj commented Dec 5, 2017 •

edited

Loading

alexcrichton commented Dec 5, 2017

cramertj commented Dec 5, 2017 •

edited

Loading

alexcrichton commented Dec 5, 2017

cramertj commented Dec 5, 2017 •

edited

Loading

alexcrichton commented Dec 5, 2017

Tweak the PollEvented::deregister signature #55

Tweak the PollEvented::deregister signature #55

Conversation

alexcrichton commented Dec 5, 2017 • edited Loading

cramertj commented Dec 5, 2017 • edited Loading

alexcrichton commented Dec 5, 2017

cramertj commented Dec 5, 2017 • edited Loading

alexcrichton commented Dec 5, 2017

cramertj commented Dec 5, 2017 • edited Loading

alexcrichton commented Dec 5, 2017

Tweak the `PollEvented::deregister` signature #55

Tweak the `PollEvented::deregister` signature #55

alexcrichton commented Dec 5, 2017 •

edited

Loading

cramertj commented Dec 5, 2017 •

edited

Loading

cramertj commented Dec 5, 2017 •

edited

Loading

cramertj commented Dec 5, 2017 •

edited

Loading